Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add tidb-ctl config #236

Merged
merged 2 commits into from
Feb 7, 2024
Merged

feat: add tidb-ctl config #236

merged 2 commits into from
Feb 7, 2024

Conversation

jayl1e
Copy link
Contributor

@jayl1e jayl1e commented Feb 7, 2024

Why

  • add tidb-ctl build config

Signed-off-by: lijie <lijie@pingcap.com>
Copy link

ti-chi-bot bot commented Feb 7, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Key changes:

The pull request adds a builders section to the packages.yaml.tmpl file, which includes a Docker image for building tidb-ctl. It also updates the steps section to use make instead of go build. Finally, it changes the path for the tidb-ctl executable in the artifacts section.

Potential problems:

  • The comment in the builders section mentions that tidb-ctl has not release branches and all tags tagged on the master branch, but this is not addressed in the changes made in this pull request. This could cause problems down the line if the tidb-ctl build process changes in a way that makes it incompatible with the current Docker image.
  • The change to use make instead of go build is not explained in the pull request description. It would be helpful to know why this change was made and if there were any issues with the previous build process.
  • The change to the path for the tidb-ctl executable could cause issues if other parts of the build process were expecting it to be in a different location.

Fixing suggestions:

  • Address the issue with tidb-ctl not having release branches and all tags being tagged on the master branch. This could involve creating release branches for tidb-ctl or finding a different way to handle the build process.
  • Provide an explanation for the change from go build to make.
  • Consider whether changing the path for the tidb-ctl executable is necessary and if there could be any unintended consequences. If the change is necessary, make sure to update any other parts of the build process that rely on the old path.

@ti-chi-bot ti-chi-bot bot added the size/S label Feb 7, 2024
Copy link

ti-chi-bot bot commented Feb 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

ti-chi-bot bot commented Feb 7, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-02-07 10:20:52.722128002 +0000 UTC m=+352178.288897900: ☑️ agreed by wuhuizuo.

@ti-chi-bot ti-chi-bot bot merged commit ee6dfd8 into main Feb 7, 2024
2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feat/tidb-ctl branch February 7, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants